Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix vite watch mode dep optimizer #1876

Closed
wants to merge 5 commits into from
Closed

fix vite watch mode dep optimizer #1876

wants to merge 5 commits into from

Conversation

patricklx
Copy link
Contributor

@patricklx patricklx commented Apr 10, 2024 •
edited
Loading

  • all deps are optimized
  • initial dep scan creates all deps
  • all optimized chunks are used
  • new deps are optimized
  • different specifiers use same optimized id
  • addon rebuilds are newly optimized

dep optimizer has multiple phases

  • Scanning, which uses the vite plugins and configured rollup plugins .
  • pre bundle, does not use configured rollup plugins.

This is the order

  • inital scan
  • initial scan must provide correct import specifier to vite import analysis plugin, this is probably because we cannot use vite own alias handling
  • pre bundling
  • vite starts and gets requests
  • vite checks if some need optimization
  • rules:
  1. Specifier must be bare import
  2. Importer cannot be in node modules
  3. Needs to be node_resolveable, so follow node resolution without additional plugin resolution
  • if a new dep is detected, vite schedules a new scan

Currently this is broken because of 2 things. app is in rewritten-app. It's actually easy to fix by changing to importer to the original app. Only needed when resolving bare imports.

The more difficult one is rewritten-packages. They must be node resolvable from original app. They are not currently.
I suggest to create a link from @embroider/rewritten-packages to the one inside . embroider and rewrite specifiers accordingly.

It currently looks like it's working, because the initial scan detects the deps. But any new dep added later will not be optimized.

fixes:

  • rewrite rewritten-app to original app location
  • link rewritten-package to be node resolvable
  • esbuild scan needs to pass renamed modules and appjsmatch to vite import analysis

@patricklx patricklx force-pushed the fix-vite-dep-optimizer branch 3 times, most recently from 495565b to e5c7c9e Compare April 11, 2024 20:54
@ef4
Copy link
Contributor

ef4 commented Apr 11, 2024

I suggest to create a link from @embroider/rewritten-packages to the one inside

This is how things used to work. Doing it correctly requires more than just links to the rewritten packages, in general it requires you to rewrite the entire node_modules graph in order to make every package see all the correct dependencies.

And I don't think it's necessary. I think that from the perspective of the rewritten-app, the rewritten-packages don't appear to be inside node_modules. But when we stop rewriting the app, they will.

@patricklx
Copy link
Contributor Author

patricklx commented Apr 12, 2024 •
edited
Loading

I suggest to create a link from @embroider/rewritten-packages to the one inside

This is how things used to work. Doing it correctly requires more than just links to the rewritten packages, in general it requires you to rewrite the entire node_modules graph in order to make every package see all the correct dependencies.

And I don't think it's necessary. I think that from the perspective of the rewritten-app, the rewritten-packages don't appear to be inside node_modules. But when we stop rewriting the app, they will.

The problem is that vite needs a bare import and the importer cannot be in node modules.
What I'm aiming to do here is to create a resolved bare specifier.
So instead of having ./node_modules/.embroider/rewritten-packages/pgk1/node_mod/pkg/my-file.js
We have
@embroider/rewritten-packages/pgk1/node_mod/pkg/my-file.js
And things will work

What we are currently doing for resolving rewritten-packages is doing a rehome inside the rewritten-packages, onto moved-target.js https://github.com/embroider-build/embroider/blob/main/packages/core/src/module-resolver.ts#L954

And here is how vite checks if it should optimise:

  • Specifier must be bare import
  • Importer cannot be in node modules
  • Needs to be node_resolveable, so follow node resolution without additional plugin resolution

When we are doing a rehome, the importer is in node_modules... So rhat doesn't work

https://github.com/vitejs/vite/blob/6c323d5b3ab3cdf81d21bbe965ed3c36aa7f0589/packages/vite/src/node/plugins/resolve.ts#L868
and
https://github.com/vitejs/vite/blob/6c323d5b3ab3cdf81d21bbe965ed3c36aa7f0589/packages/vite/src/node/plugins/resolve.ts#L353

So it's not a problem of rewritten-app.

@patricklx patricklx force-pushed the fix-vite-dep-optimizer branch 16 times, most recently from 6550053 to 41ac14e Compare April 13, 2024 16:09
patricklx
patricklx commented Apr 13, 2024
View reviewed changes
packages/core/src/module-resolver.ts Outdated
return resolution;
case 'ignored':
case 'not_found':
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignored should fallback resolve

patricklx
patricklx commented Apr 13, 2024
View reviewed changes
packages/shared-internals/src/package.ts Outdated
@@ -25,7 +25,11 @@ export default class Package {

@Memoize()
protected get internalPackageJSON() {
return JSON.parse(readFileSync(join(this.root, 'package.json'), 'utf8'));
try {
return JSON.parse(readFileSync(join(this.root, 'package.json'), 'utf8'));
Copy link
Contributor Author

@patricklx patricklx Apr 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workaround for fromFile pointing at /app/tests/package.json and vite/deps

@patricklx patricklx force-pushed the fix-vite-dep-optimizer branch 7 times, most recently from c6e0aa3 to a6a07a4 Compare April 15, 2024 11:07
@patricklx patricklx force-pushed the fix-vite-dep-optimizer branch 2 times, most recently from 7768bde to cde544d Compare June 20, 2024 13:21
@patricklx patricklx closed this Jun 20, 2024
@patricklx patricklx reopened this Jun 20, 2024
mansona
mansona requested changes Jul 29, 2024
View reviewed changes
Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should be split up into individual PRs that address single issues that are verifiable with tests. Especially now that #2029 is merged we should be methodical with how we proceed

@patricklx patricklx force-pushed the fix-vite-dep-optimizer branch 2 times, most recently from 1854736 to e7bd441 Compare July 31, 2024 09:24
@patricklx patricklx mentioned this pull request Aug 28, 2024
Closed
@patricklx patricklx force-pushed the fix-vite-dep-optimizer branch 2 times, most recently from 9c9c6c6 to 101a4b1 Compare September 12, 2024 21:01
@patricklx patricklx changed the title fix vite dep optimizer fix vite watch mode dep optimizer Sep 13, 2024
@patricklx patricklx mentioned this pull request Sep 14, 2024
Closed
@patricklx patricklx force-pushed the fix-vite-dep-optimizer branch 6 times, most recently from af4648d to 3e13a8d Compare October 1, 2024 11:58
@ef4 ef4 mentioned this pull request Oct 2, 2024
Merged
@patricklx
Copy link
Contributor Author

fixed in #2135

@patricklx patricklx closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mansona mansona mansona requested changes

Assignees
No one assigned
Labels
blocked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@patricklx @ef4 @mansona